Skip to content

[Bugfix] Fix merging logic to append multiple jvm/otlp configurations #1870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Sep 23, 2025
Merged

[Bugfix] Fix merging logic to append multiple jvm/otlp configurations #1870

merged 14 commits into from
Sep 23, 2025

Conversation

@lisguo
Copy link
Contributor

@lisguo lisguo commented Sep 18, 2025 •
edited
Loading

Description of the issue

Currently if you try to use the "append-config" action on two configurations that have separate JVM configurations, it will fail.

Kafka Config:

{
  "metrics": {
    "namespace": "CWAgent",
    "metrics_collected": {
      "jmx": [
        {
          "endpoint": "localhost:1111",
          "kafka": {
            "measurement": [
              "kafka.request.time.avg",
            ]
          },
          "append_dimensions": {
            "ClusterName": "MyKafkaCluster"
          }
        },
        {
          "endpoint": "localhost:1111",
          "jvm": {
            "measurement": [
              "jvm.classes.loaded",
            ]
          },
          "append_dimensions": {
            "ProcessGroupName": "MyKafkaCluster"
          }
        }
      ]
    }
  }
}

Tomcat config:

{
  "metrics": {
    "namespace": "CWAgent",
    "metrics_collected": {
      "jmx": [
        {
          "endpoint": "localhost:2222",
          "tomcat": {
            "measurement": [
              "tomcat.sessions",
            ]
          },
          "append_dimensions": {
            "AppName": "MyTomcatServer"
          }
        },
        {
          "endpoint": "localhost:2222",
          "jvm": {
            "measurement": [
              "jvm.classes.loaded",
            ]
          },
          "append_dimensions": {
            "ProcessGroupName": "MyTomcatServer"
          }
        }
      ]
    }
  }
}

I expect this to merge the jvm configurations to this valid config:

{
  "metrics": {
    "namespace": "CWAgent",
    "metrics_collected": {
      "jmx": [
        {
          "endpoint": "localhost:1111",
          "kafka": {
            "measurement": [
              "kafka.request.time.avg",
            ]
          },
          "append_dimensions": {
            "ClusterName": "MyKafkaCluster"
          }
        },
        {
          "endpoint": "localhost:1111",
          "jvm": {
            "measurement": [
              "jvm.classes.loaded",
            ]
          },
          "append_dimensions": {
            "ProcessGroupName": "MyKafkaCluster"
          }
        },
        {
          "endpoint": "localhost:2222",
          "tomcat": {
            "measurement": [
              "tomcat.sessions",
            ]
          },
          "append_dimensions": {
            "AppName": "MyTomcatServer"
          }
        },
        {
          "endpoint": "localhost:2222",
          "jvm": {
            "measurement": [
              "jvm.classes.loaded",
            ]
          },
          "append_dimensions": {
            "ProcessGroupName": "MyTomcatServer"
          }
        }
      ]
    }
  }
}

Description of changes

Modify the mergeMap function in the translator to add a conditional if we see a "jmx" or "otlp" configuration, then merge the incoming jmx config into the list.

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Added unit test and manual testing. I appended 4 jmx configurations and see the otel receivers populated:

receivers:
    jmx/0:
        collection_interval: 1m0s
        endpoint: localhost:1111
        jar_path: /opt/aws/amazon-cloudwatch-agent/bin/opentelemetry-jmx-metrics.jar
        otlp:
            endpoint: 0.0.0.0:0
            timeout: 5s
        resource_attributes:
            ProcessGroupName: CinchJVM
        target_system: jvm
    jmx/1:
        collection_interval: 1m0s
        endpoint: localhost:2222
        jar_path: /opt/aws/amazon-cloudwatch-agent/bin/opentelemetry-jmx-metrics.jar
        otlp:
            endpoint: 0.0.0.0:0
            timeout: 5s
        resource_attributes:
            ClusterName: MyKafkaCluster
        target_system: kafka
    jmx/2:
        collection_interval: 1m0s
        endpoint: localhost:2222
        jar_path: /opt/aws/amazon-cloudwatch-agent/bin/opentelemetry-jmx-metrics.jar
        otlp:
            endpoint: 0.0.0.0:0
            timeout: 5s
        resource_attributes:
            ProcessGroupName: MyKafkaCluster
        target_system: jvm
    jmx/3:
        collection_interval: 1m0s
        endpoint: localhost:3333
        jar_path: /opt/aws/amazon-cloudwatch-agent/bin/opentelemetry-jmx-metrics.jar
        otlp:
            endpoint: 0.0.0.0:0
            timeout: 5s
        resource_attributes:
            AppName: MyTomcatServer
        target_system: tomcat
    jmx/4:
        collection_interval: 1m0s
        endpoint: localhost:3333
        jar_path: /opt/aws/amazon-cloudwatch-agent/bin/opentelemetry-jmx-metrics.jar
        otlp:
            endpoint: 0.0.0.0:0
            timeout: 5s
        resource_attributes:
            ProcessGroupName: MyTomcatServer
        target_system: jvm

Requirements

Before commiting your code, please do the following steps.

  1. Run make fmt and make fmt-sh
  2. Run make lint

Integration Tests

To run integration tests against this PR, add the ready for testing label.

@lisguo lisguo requested a review from a team as a code owner September 18, 2025 21:22
@lisguo lisguo changed the title [Bugfix] Fix JVM merging logic to append multiple jvm configurations [Bugfix] Fix JMX merging logic to append multiple jvm configurations Sep 18, 2025
musa-asad
musa-asad reviewed Sep 18, 2025
View reviewed changes
musa-asad
musa-asad reviewed Sep 18, 2025
View reviewed changes
@lisguo lisguo added the ready for testing Indicates this PR is ready for integration tests to run label Sep 21, 2025
musa-asad
musa-asad previously approved these changes Sep 21, 2025
View reviewed changes
@lisguo
Copy link
Contributor Author

lisguo commented Sep 22, 2025

need to fix integ test

@lisguo lisguo changed the title [Bugfix] Fix JMX merging logic to append multiple jvm configurations [Bugfix] Fix merging logic to append multiple jvm/otlp configurations Sep 22, 2025
lisguo
lisguo commented Sep 22, 2025
View reviewed changes
musa-asad
musa-asad reviewed Sep 22, 2025
View reviewed changes
musa-asad
musa-asad reviewed Sep 22, 2025
View reviewed changes
jefchien
jefchien previously approved these changes Sep 22, 2025
View reviewed changes
jefchien
jefchien previously approved these changes Sep 22, 2025
View reviewed changes
translator/jsonconfig/mergeJsonUtil/util_test.go
"github.com/stretchr/testify/assert"
)

func TestMergeArrayOrObjectConfiguration(t *testing.T) {
Copy link
Contributor

@jefchien jefchien Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Missing coverage for merging an array into an object.

musa-asad
musa-asad previously approved these changes Sep 22, 2025
View reviewed changes
@lisguo lisguo dismissed stale reviews from musa-asad and jefchien via 0d3e0a7 September 23, 2025 14:33
@lisguo lisguo merged commit 9351351 into main Sep 23, 2025
171 of 186 checks passed
@lisguo lisguo deleted the lisaguo-jvm-merge branch September 23, 2025 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@musa-asad musa-asad musa-asad left review comments

@JayPolanco JayPolanco JayPolanco approved these changes

@jefchien jefchien jefchien approved these changes

Assignees

No one assigned

Labels

ready for testing Indicates this PR is ready for integration tests to run

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lisguo @JayPolanco @jefchien @musa-asad